Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add debug_assert_ne + assert_ne #35074

Merged
merged 1 commit into from
Sep 21, 2016
Merged

Conversation

ashleygwilliams
Copy link
Member

@ashleygwilliams ashleygwilliams commented Jul 27, 2016

✨ work in progress, please do not merge ✨

fixes #35073

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ashleygwilliams ashleygwilliams changed the title [WIP] add debug_assert_ne [WIP] add debug_assert_ne + assert_ne Jul 27, 2016
/// assert_ne!(a, b);
/// ```
#[macro_export]
///#[stable(feature = "????", since = "????")]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what to put here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

macros are a bit odd, since they become insta-stable. Right now, since should be 1.12.0. Unsure about feature, since it won't have one. @alexcrichton ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the feature effectively doesn't matter -- assert_ne would be fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(what @aturon said)

@ashleygwilliams
Copy link
Member Author

hi! does this need tests? i'm struggling to find where assert_eq is tested. it is not in the same file, and a search of this repo for assert_eq and test leads to a lot of results 😬

would love some direction on where/if to write tests. otherwise i think this should be good to go!

@ashleygwilliams
Copy link
Member Author

ok! tests added. thanks for the point in the right direction @steveklabnik 🐬

@ashleygwilliams ashleygwilliams changed the title [WIP] add debug_assert_ne + assert_ne add debug_assert_ne + assert_ne Aug 17, 2016
@@ -0,0 +1,12 @@
#[derive(PartialEq, Debug)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may need our standard license header to pass make tidy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, @steveklabnik said it didn't, but i'll add it

@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Aug 31, 2016
@alexcrichton
Copy link
Member

@bors: r+ 3f404f40d01f8a3f03d2ad219f81cb236d416f15

@ashleygwilliams
Copy link
Member Author

lemme know if you'd like me to squash ;)

@bors
Copy link
Contributor

bors commented Aug 31, 2016

⌛ Testing commit 3f404f4 with merge aa5ce78...

@bors
Copy link
Contributor

bors commented Aug 31, 2016

💔 Test failed - auto-linux-64-nopt-t

@alexcrichton
Copy link
Member

@ashleygwilliams sure! Looks like the stability attributes may also need to be uncommented?

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 1, 2016

📌 Commit ec8a8ef has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Sep 1, 2016

⌛ Testing commit ec8a8ef with merge 8e68479...

@bors
Copy link
Contributor

bors commented Sep 1, 2016

💔 Test failed - auto-mac-64-opt-rustbuild

@steveklabnik
Copy link
Member

Hmm, I don't see why this failed.

@alexcrichton
Copy link
Member

Hm I guess maybe that could be a bug in rustbuild where it's using the wrong standard library, but I'd be surprised if that just came up!

@ashleygwilliams does make check-stage2-rpass pass locally for you?

@alexcrichton
Copy link
Member

@ashleygwilliams of did you pass --enable-rustbuild to configure? In that case the command would be:

python src/bootstrap/bootstrap.py --step check-rpass 

@ashleygwilliams
Copy link
Member Author

am waiting on it now @alexcrichton

@ashleygwilliams
Copy link
Member Author

fails the same on my local machine as it fails in travis.

@alexcrichton
Copy link
Member

Oh right these macros are defined in libcore but need to be reexported from the standard library

@ashleygwilliams
Copy link
Member Author

lol omg I KNEW IT. i had been bugging @steveklabnik about needing to "register" the macros somewhere. commit + squash coming up

@@ -302,7 +302,7 @@ use prelude::v1::*;
// We want to reexport a few macros from core but libcore has already been
// imported by the compiler (via our #[no_std] attribute) In this case we just
// add a new crate name so we can attach the reexports to it.
#[macro_reexport(assert, assert_eq, debug_assert, debug_assert_eq,
#[macro_reexport(assert, assert_eq, assert_ne, debug_assert, debug_assert_eq,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

debug_assert_ne as well?

@ashleygwilliams
Copy link
Member Author

test result: ok. 2445 passed; 0 failed; 60 ignored; 0 measured
The command "docker run -v `pwd`:/build rust sh -c " ./configure --llvm-root=/usr/lib/llvm-3.7 && make tidy && make check-notidy -j4 "" exited with 2.
Done. Your build exited with 1.

i am unclear what this means.

@alexcrichton
Copy link
Member

@bors: r+

Ah yeah that's fine, travis is broken right now unfortunately :(

@bors
Copy link
Contributor

bors commented Sep 2, 2016

📌 Commit 01d085b has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Sep 2, 2016

⌛ Testing commit 01d085b with merge 848e652...

@bors
Copy link
Contributor

bors commented Sep 3, 2016

💔 Test failed - auto-linux-32-nopt-t

@steveklabnik
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Sep 3, 2016

🔒 Merge conflict

@steveklabnik
Copy link
Member

/cc @ashleygwilliams , let's get that merge conflict fixed

@steveklabnik
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 21, 2016

📌 Commit 3d8d557 has been approved by steveklabnik

@steveklabnik
Copy link
Member

Homu still seems to think this isn't mergeable @alexcrichton :/

@steveklabnik
Copy link
Member

Actually, it appears it was just a bad cache, it's working now 👍

@bors
Copy link
Contributor

bors commented Sep 21, 2016

⌛ Testing commit 3d8d557 with merge 53f9730...

bors added a commit that referenced this pull request Sep 21, 2016
add debug_assert_ne + assert_ne

~~✨ work in progress, please do not merge ✨~~

fixes #35073
@bors bors merged commit 3d8d557 into rust-lang:master Sep 21, 2016
kumbayo added a commit to kumbayo/intellij-rust that referenced this pull request Oct 7, 2016
This improves navigation and code highlighting inside the following macros:
assert!(a == b);
debug_assert!(a == b);
assert_eq!(a, b);
debug_assert_eq!(a, b);
assert_ne!(a, b);
debug_assert_ne!(a, b);

For assert! and debug_assert! format arguments are supported:
assert!(a == b, "Some text");
assert!(a == b, "Text {} {} syntax", "with", "format");

Different parenthesis are supported:
assert!(a == b);
assert![a == b];
assert!{a == b};

Fixes intellij-rust#636

assert_ne! is stable since Rust 1.12.
rust-lang/rust#35074
kumbayo added a commit to kumbayo/intellij-rust that referenced this pull request Oct 7, 2016
This improves navigation and code highlighting inside the following macros:
assert!(a == b);
debug_assert!(a == b);
assert_eq!(a, b);
debug_assert_eq!(a, b);
assert_ne!(a, b);
debug_assert_ne!(a, b);

Format arguments are supported:
assert!(a == b, "Text {} {} syntax", "with", "format");
assert!(a == b, "Some text");
assert_eq!(a, b, "Some text");
assert_ne!(a, b, "Some text");

Different parenthesis are supported:
assert!(a == b);
assert![a == b];
assert!{a == b};

Fixes intellij-rust#636

assert_ne! is stable since Rust 1.12.
rust-lang/rust#35074
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for the assert_ne and debug_assert_ne macros
7 participants